Skip to content

feat: support sections and headers in RAC gridlist #8667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

yihuiliao
Copy link
Member

@yihuiliao yihuiliao commented Aug 1, 2025

Closes

btw dnd it not supported in this pr, that'll be follow-up when we bump up sections to beta/rc/ga (one of them)

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

test using the two added stories in RAC. one is virtualized + dynamic, the other is non-virtualized static

🧢 Your Project:

@rspbot
Copy link

rspbot commented Aug 1, 2025

@rspbot
Copy link

rspbot commented Aug 2, 2025

@yihuiliao yihuiliao marked this pull request as ready for review August 5, 2025 00:01
* See `useGridList` for more details about grid list.
* @param props - Props for the section.
*/
export function useGridListSection(props: AriaGridListSectionProps): GridListSectionAria {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass state and ref, we always seem to regret not passing them and it's breaking to add them later

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could pass it but wouldn't they be unused?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still need to declare them in the signature so that others using the hook know they should supply them. that's the only way it can be non-breaking


const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null);

export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with ListBox, you can just use a normal Header, but with GridList, in order to follow correct aria-pattern, the header must be inside a div with a role=row, hence why i've created a new component called GridListHeader. are we okay with that?

from WAI-ARIA:

Each cell is either a DOM descendant of or owned by a row element and has one of the following roles:

  • columnheader if the cell contains a title or header information for the column.
  • rowheader if the cell contains title or header information for the row.
  • gridcell if the cell does not contain column or row header information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, but the row itself should get aria-attributes like aria-rowindex when virtualized if so.

@rspbot
Copy link

rspbot commented Aug 5, 2025


return (
<div {...rowProps} >
<header className="react-aria-Header" {...props} ref={ref}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having two elements will make this harder to style. Maybe we can use display: 'contents' on the inner one so we have the correct ARIA structure, but you only need to style the outer one:

<header {...rowProps} ref={ref}>
  <div {...rowHeaderProps} style={{display: 'contents'}}>
    {children}
  </div>
</header>

This would match the structure of GridListItem too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linking related discussion #8667 (comment)

I think I like the display contents better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I did the same for the load more elements too (i.e. TableLoadMoreItem)

Copy link
Contributor

@nwidynski nwidynski Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snowystinger Regarding #8667 (comment)

I think we could provide a custom CollectionNode to wrap headers children with the row div.

Otherwise we could expose an internal context in Header to be set by the GridList? I think its worth to try and maintain a universal API if somehow possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could provide a custom #8523 to wrap headers children with the row div.

Definitely an interesting idea. Though I imagine that would have the same issue with styling, so we'd probably still use 'display: contents', or possibly I haven't thought through your other PR enough yet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, styling issue persists. This just enables re-use of Header.

Copy link
Member Author

@yihuiliao yihuiliao Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've decided as a team to go ahead and use GridListHeader since it's the most straightforward in terms of implementation. While the re-use of Header would be nice, with the exception of ListBox, we do have a history in RAC of having collection specific components ListBoxSection vs. GridListSection, or ListBoxItem vs GridListItem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with that 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so we have the history, it was noted that we'd need a custom renderer as well in order to reuse the Header in this way.

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still need to test the screenreader behavior but just some things I noted when scanning the code


const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null);

export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, but the row itself should get aria-attributes like aria-rowindex when virtualized if so.

@rspbot
Copy link

rspbot commented Aug 8, 2025

@rspbot
Copy link

rspbot commented Aug 12, 2025

@rspbot
Copy link

rspbot commented Aug 13, 2025

let {collection} = state;
let nodes = [...collection];
// TODO: refactor ListCollection to store an absolute index of a node's position?
rowProps['aria-rowindex'] = nodes.find(node => node.type === 'section') ? [...collection.getKeys()].filter((key) => collection.getItem(key)?.type !== 'section').findIndex((key) => key === node.key) + 1 : node.index + 1;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did end up changing how we calculate rowIndex for rows inside Sections since Safari VO would read out the rows incorrectly otherwise. for example, say you were on the first item in Section 3, instead of saying something like "row 8 of 12" it would say "row 1 of 12"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if it would reasonable to use aria-posinset like tree does, did you try that before?

Copy link
Member Author

@yihuiliao yihuiliao Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way we calculate posinset for tree is also node.index + 1 so i'm not sure if that would be any different. either way, i think the calculation for the index/posinset will need to be adjusted unless you mean something else?

@rspbot
Copy link

rspbot commented Aug 13, 2025

@rspbot
Copy link

rspbot commented Aug 13, 2025

## API Changes

react-aria-components

/react-aria-components:Popover

 Popover {
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   arrowBoundaryOffset?: number = 0
-  arrowRef?: RefObject<Element | null>
   boundaryElement?: Element = document.body
   children?: ChildrenOrFunction<PopoverRenderProps>
   className?: ClassNameOrFunction<PopoverRenderProps>
   containerPadding?: number = 12
   defaultOpen?: boolean
   isEntering?: boolean
   isExiting?: boolean
   isKeyboardDismissDisabled?: boolean = false
   isNonModal?: boolean
   isOpen?: boolean
   maxHeight?: number
   offset?: number = 8
   onOpenChange?: (boolean) => void
   placement?: Placement = 'bottom'
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldCloseOnInteractOutside?: (Element) => boolean
   shouldFlip?: boolean = true
   shouldUpdatePosition?: boolean = true
   slot?: string | null
   style?: StyleOrFunction<PopoverRenderProps>
   trigger?: string
   triggerRef?: RefObject<Element | null>
 }

/react-aria-components:VisuallyHidden

-VisuallyHidden {
-  children?: ReactNode
-  className?: string | undefined
-  elementType?: string | JSXElementConstructor<any> = 'div'
-  id?: string | undefined
-  isFocusable?: boolean
-  role?: AriaRole | undefined
-  style?: CSSProperties | undefined
-  tabIndex?: number | undefined
-}

/react-aria-components:PopoverProps

 PopoverProps {
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   arrowBoundaryOffset?: number = 0
-  arrowRef?: RefObject<Element | null>
   boundaryElement?: Element = document.body
   children?: ChildrenOrFunction<PopoverRenderProps>
   className?: ClassNameOrFunction<PopoverRenderProps>
   containerPadding?: number = 12
   defaultOpen?: boolean
   isEntering?: boolean
   isExiting?: boolean
   isKeyboardDismissDisabled?: boolean = false
   isNonModal?: boolean
   isOpen?: boolean
   maxHeight?: number
   offset?: number = 8
   onOpenChange?: (boolean) => void
   placement?: Placement = 'bottom'
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldCloseOnInteractOutside?: (Element) => boolean
   shouldFlip?: boolean = true
   shouldUpdatePosition?: boolean = true
   slot?: string | null
   style?: StyleOrFunction<PopoverRenderProps>
   trigger?: string
   triggerRef?: RefObject<Element | null>
 }

/react-aria-components:GridLayoutOptions

 GridLayoutOptions {
   dropIndicatorThickness?: number = 2
   maxColumns?: number = Infinity
-  maxHorizontalSpace?: number = Infinity
   maxItemSize?: Size = Infinity
   minItemSize?: Size = 200 x 200
   minSpace?: Size = 18 x 18
   preserveAspectRatio?: boolean = false

/react-aria-components:GridListHeader

+GridListHeader {
+  UNTYPED
+}

/react-aria-components:GridListSection

+GridListSection <T extends {}> {
+  aria-label?: string
+  children?: ReactNode | ({}) => ReactElement
+  className?: string
+  dependencies?: ReadonlyArray<any>
+  id?: Key
+  items?: Iterable<{}>
+  style?: CSSProperties
+  value?: {}
+}

@internationalized/date

/@internationalized/date:setLocalTimeZone

-setLocalTimeZone {
-  timeZone: string
-  returnVal: undefined
-}

/@internationalized/date:resetLocalTimeZone

-resetLocalTimeZone {
-  returnVal: undefined
-}

@react-aria/gridlist

/@react-aria/gridlist:useGridListSection

+useGridListSection <T> {
+  props: AriaGridListSectionProps
+  state: ListState<T>
+  ref: RefObject<HTMLElement | null>
+  returnVal: undefined
+}

@react-aria/i18n

/@react-aria/i18n:isRTL

-isRTL {
-  localeString: string
-  returnVal: undefined
-}

@react-aria/overlays

/@react-aria/overlays:AriaPositionProps

 AriaPositionProps {
   arrowBoundaryOffset?: number = 0
-  arrowRef?: RefObject<Element | null>
   arrowSize?: number = 0
   boundaryElement?: Element = document.body
   containerPadding?: number = 12
   crossOffset?: number = 0
   maxHeight?: number
   offset?: number = 0
   onClose?: () => void | null
   overlayRef: RefObject<Element | null>
   placement?: Placement = 'bottom'
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldFlip?: boolean = true
   shouldUpdatePosition?: boolean = true
   targetRef: RefObject<Element | null>
 }

/@react-aria/overlays:PositionAria

 PositionAria {
   arrowProps: DOMAttributes
   overlayProps: DOMAttributes
   placement: PlacementAxis | null
-  triggerOrigin: {
-    x: number
-  y: number
-} | null
   updatePosition: () => void
 }

/@react-aria/overlays:AriaPopoverProps

 AriaPopoverProps {
   arrowBoundaryOffset?: number = 0
-  arrowRef?: RefObject<Element | null>
   arrowSize?: number = 0
   boundaryElement?: Element = document.body
   containerPadding?: number = 12
   crossOffset?: number = 0
   isKeyboardDismissDisabled?: boolean = false
   isNonModal?: boolean
   maxHeight?: number
   offset?: number = 0
   placement?: Placement = 'bottom'
   popoverRef: RefObject<Element | null>
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldCloseOnInteractOutside?: (Element) => boolean
   shouldFlip?: boolean = true
   shouldUpdatePosition?: boolean = true
   triggerRef: RefObject<Element | null>
 }

/@react-aria/overlays:PopoverAria

 PopoverAria {
   arrowProps: DOMAttributes
   placement: PlacementAxis | null
   popoverProps: DOMAttributes
-  triggerOrigin: {
-    x: number
-  y: number
-} | null
   underlayProps: DOMAttributes
 }

@react-spectrum/overlays

/@react-spectrum/overlays:Popover

 Popover {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
   arrowBoundaryOffset?: number = 0
-  arrowRef?: RefObject<Element | null>
   arrowSize?: number = 0
   bottom?: Responsive<DimensionValue>
   boundaryElement?: Element = document.body
   children: ReactNode
   containerPadding?: number = 12
   crossOffset?: number = 0
   disableFocusManagement?: boolean
   enableBothDismissButtons?: boolean
   end?: Responsive<DimensionValue>
   flex?: Responsive<string | number | boolean>
   flexBasis?: Responsive<number | string>
   flexGrow?: Responsive<number>
   flexShrink?: Responsive<number>
   gridArea?: Responsive<string>
   gridColumn?: Responsive<string>
   gridColumnEnd?: Responsive<string>
   gridColumnStart?: Responsive<string>
   gridRow?: Responsive<string>
   gridRowEnd?: Responsive<string>
   gridRowStart?: Responsive<string>
   groupRef?: RefObject<Element | null>
   height?: Responsive<DimensionValue>
   hideArrow?: boolean
   isDisabled?: boolean
   isHidden?: Responsive<boolean>
   isKeyboardDismissDisabled?: boolean = false
   isNonModal?: boolean
   justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
   left?: Responsive<DimensionValue>
   margin?: Responsive<DimensionValue>
   marginBottom?: Responsive<DimensionValue>
   marginEnd?: Responsive<DimensionValue>
   marginStart?: Responsive<DimensionValue>
   marginTop?: Responsive<DimensionValue>
   marginX?: Responsive<DimensionValue>
   marginY?: Responsive<DimensionValue>
   maxHeight?: Responsive<DimensionValue>
   maxWidth?: Responsive<DimensionValue>
   minHeight?: Responsive<DimensionValue>
   minWidth?: Responsive<DimensionValue>
   offset?: number = 0
   onBlurWithin?: (FocusEvent) => void
   onDismissButtonPress?: () => void
   onEnter?: () => void
   onEntered?: () => void
   onEntering?: () => void
   onExit?: () => void
   onExited?: () => void
   onExiting?: () => void
   onFocusWithin?: (FocusEvent) => void
   onFocusWithinChange?: (boolean) => void
   order?: Responsive<number>
   placement?: Placement = 'bottom'
   position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
   right?: Responsive<DimensionValue>
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldCloseOnInteractOutside?: (Element) => boolean
   shouldContainFocus?: boolean
   shouldFlip?: boolean = true
   shouldUpdatePosition?: boolean = true
   start?: Responsive<DimensionValue>
   state: OverlayTriggerState
   top?: Responsive<DimensionValue>
   triggerRef: RefObject<Element | null>
   width?: Responsive<DimensionValue>
   zIndex?: Responsive<number>
 }

@react-spectrum/s2

/@react-spectrum/s2:PopoverProps

 PopoverProps {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
-  arrowRef?: RefObject<Element | null>
   boundaryElement?: Element = document.body
   children?: ChildrenOrFunction<PopoverRenderProps>
   className?: ClassNameOrFunction<PopoverRenderProps>
   containerPadding?: number = 12
   defaultOpen?: boolean
   hideArrow?: boolean = false
   isEntering?: boolean
   isExiting?: boolean
   isOpen?: boolean
   maxHeight?: number
   offset?: number = 8
   onOpenChange?: (boolean) => void
   placement?: Placement = 'bottom'
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldFlip?: boolean = true
   size?: 'S' | 'M' | 'L'
   slot?: string | null
   style?: StyleOrFunction<PopoverRenderProps>
   styles?: StyleString
   trigger?: string
   triggerRef?: RefObject<Element | null>
 }

@react-stately/data

/@react-stately/data:ListData

 ListData <T> {
-  addKeysToSelection: (Selection) => void
   append: (Array<T>) => void
   filterText: string
   getItem: (Key) => T | undefined
   insert: (number, Array<T>) => void
   insertAfter: (Key, Array<T>) => void
   insertBefore: (Key, Array<T>) => void
   items: Array<T>
   move: (Key, number) => void
   moveAfter: (Key, Iterable<Key>) => void
   moveBefore: (Key, Iterable<Key>) => void
   prepend: (Array<T>) => void
   remove: (Array<Key>) => void
-  removeKeysFromSelection: (Selection) => void
   removeSelectedItems: () => void
   selectedKeys: Selection
   setFilterText: (string) => void
   setSelectedKeys: (Selection) => void
 }

/@react-stately/data:AsyncListData

 AsyncListData <T> {
-  addKeysToSelection: (Selection) => void
   append: (Array<T>) => void
   error?: Error
   filterText: string
   getItem: (Key) => T | undefined
   insert: (number, Array<T>) => void
   insertAfter: (Key, Array<T>) => void
   insertBefore: (Key, Array<T>) => void
   isLoading: boolean
   items: Array<T>
   loadMore: () => void
   loadingState: LoadingState
   move: (Key, number) => void
   moveAfter: (Key, Iterable<Key>) => void
   moveBefore: (Key, Iterable<Key>) => void
   prepend: (Array<T>) => void
   reload: () => void
   remove: (Array<Key>) => void
-  removeKeysFromSelection: (Selection) => void
   removeSelectedItems: () => void
   selectedKeys: Selection
   setFilterText: (string) => void
   setSelectedKeys: (Selection) => void
   sortDescriptor?: SortDescriptor
   update: (Key, T) => void
 }

@react-stately/layout

/@react-stately/layout:GridLayoutOptions

 GridLayoutOptions {
   dropIndicatorThickness?: number = 2
   maxColumns?: number = Infinity
-  maxHorizontalSpace?: number = Infinity
   maxItemSize?: Size = Infinity
   minItemSize?: Size = 200 x 200
   minSpace?: Size = 18 x 18
   preserveAspectRatio?: boolean = false

});

return {
rowProps: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so ideally rowProps would have aria-rowindex but it's difficult to determine what the correct index is for the header. especially in the case where a gridlist has sections that both contain headers and don't container headers. basically, we rely on knowing the index of the previous section to determine what the index of the header should be. at the same time, we don't actually want to include sections in our calculations because they don't have the rowindex value which is what makes this tricky.

it'd be easy to determine if we calculated it inside GridListHeader but that wouldn't match any of our existing API and would sort of defeat the purpose of having this hook supply this information...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just thinking out loud (and definitely don't implement this yet), but is it possible to:

  1. Get the parent section
  2. Get section's previous key and source the previous section node
  3. Get the last child key of that section node and grab that key's node's index. That index should equal how many rows were in that section
  4. Repeat by going through each section before that and total up the total number of rows

I think the ideal way to calculate this will come from the collection nodes containing more information than they do now, include ways to get the "indexOfSameType" kind of information, something that maybe easier with some of the BaseCollection work in some of my other PRs

Copy link
Member Author

@yihuiliao yihuiliao Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i think we could just do a sum. i can look into it a bit more

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior looks good to me, JAWS and NVDA don't really seem to care about the missing row indexes on the Section headers so I think this is fine for now. Will need to test some of the more complicated use cases like DnD and make sure things like keyboard drag nav/drop indicators still work as expected

@yihuiliao
Copy link
Member Author

yihuiliao commented Aug 14, 2025

Will need to test some of the more complicated use cases like DnD and make sure things like keyboard drag nav/drop indicators still work as expected

Ah yeah, DnD does not work. I actually removed some of the DnD stuff because it didn't make sense to have it before we get it fully working. We decided sometime during standup to not support DnD for the alpha state. It also doesn't work in ListBox sections, so we'll need to get dnd to work with sections in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants